-
Notifications
You must be signed in to change notification settings - Fork 56
Add dist git branch to testing farm context #2936
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds the dist-git-branch to the Testing Farm context for downstream jobs, which is a useful addition for test environment context. The implementation is correct, and a new unit test has been added to verify the change. I've made one minor suggestion in the test file to combine multiple import statements from the same module for better code style and readability. Overall, great work!
| from packit_service.worker.helpers.testing_farm import ( | ||
| DownstreamTestingFarmJobHelper, | ||
| ) | ||
| from packit_service.worker.helpers.testing_farm import ( | ||
| TestingFarmClient as TFClient, | ||
| ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
✔️ pre-commit SUCCESS in 1m 43s |
| "tmt": { | ||
| "context": { | ||
| "distro": distro, | ||
| "dist-git-branch": self.koji_build.target, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
target isn't always equal to branch name. There can be a eln build from the rawhide branch (and other cases, but those shouldn't really be initiated by Packit, so we can ignore them).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, thanks. Will try to find a better approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've just noticed there is an alternative solution - #2935 . Is this appproach correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would say so.
Fixes: #2924